fix(security): command injection across all CLI entry points#584
Merged
Conversation
Comprehensive fix for shell injection vulnerabilities where user input (instance names, sandbox names, model names, API keys) was interpolated unsanitized into shell commands via run()/runInteractive()/execSync(). Changes: - Add shellQuote() and validateName() to runner.js as shared utilities - Replace all execSync() with execFileSync() in deploy (no shell) - Apply shellQuote() to every user-controlled variable in shell commands across nemoclaw.js, onboard.js, nim.js, policies.js - Add RFC 1123 name validation at CLI dispatch for sandbox/instance names - Fix path traversal in policies.js loadPreset() - Replace predictable temp file with mkdtempSync() - Remove duplicate shellQuote() definitions (now single source in runner.js) - 9 new test cases for shellQuote, validateName, and path traversal Supersedes #55, #110, #475, #540, #48, #171.
Import shellQuote from runner.js instead of defining a local copy. Single source of truth for shell quoting across the codebase.
Contributor
📝 WalkthroughWalkthroughCentralized shell escaping and name validation into Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Runner
participant OS
participant Remote
CLI->>Runner: validateName(name)
Runner-->>CLI: validated name / throw
CLI->>Runner: shellQuote(value)
Runner-->>CLI: quotedValue
CLI->>OS: execFileSync(cmd, [args...quotedValue...])
OS->>Remote: SSH / Docker / curl (argv-safe)
Remote-->>OS: response
OS-->>CLI: stdout / exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Telegram bridge: - Replace execSync with execFileSync for ssh-config retrieval - shellQuote message, API key, and session ID in remote command - Validate SANDBOX_NAME at startup - Use mkdtempSync for temp SSH config (not predictable path) Regression tests: - nemoclaw.js must not use execSync - Single shellQuote definition in bin/ - CLI rejects malicious sandbox names (e2e, no mocking) - telegram-bridge.js validates SANDBOX_NAME and avoids execSync
…ll-injection # Conflicts: # bin/lib/onboard.js # test/runner.test.js
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bin/lib/onboard.js (1)
453-473:⚠️ Potential issue | 🔴 CriticalQuote the
envassignments before passing them torun().Lines 463-466 still splice raw
CHAT_UI_URLandNVIDIA_API_KEYvalues into abash -cstring at Line 473. A value likeCHAT_UI_URL=http://x; touch /tmp/pwned #will execute on the host beforeopenshell sandbox createeven runs.🐛 Proposed fix
const basePolicyPath = path.join(ROOT, "nemoclaw-blueprint", "policies", "openclaw-sandbox.yaml"); const createArgs = [ - `--from "${buildCtx}/Dockerfile"`, - `--name "${sandboxName}"`, - `--policy "${basePolicyPath}"`, + `--from ${shellQuote(path.join(buildCtx, "Dockerfile"))}`, + `--name ${shellQuote(sandboxName)}`, + `--policy ${shellQuote(basePolicyPath)}`, ]; // --gpu is intentionally omitted. See comment in startGateway(). console.log(` Creating sandbox '${sandboxName}' (this takes a few minutes on first run)...`); const chatUiUrl = process.env.CHAT_UI_URL || 'http://127.0.0.1:18789'; - const envArgs = [`CHAT_UI_URL=${chatUiUrl}`]; + const envArgs = [shellQuote(`CHAT_UI_URL=${chatUiUrl}`)]; if (process.env.NVIDIA_API_KEY) { - envArgs.push(`NVIDIA_API_KEY=${process.env.NVIDIA_API_KEY}`); + envArgs.push(shellQuote(`NVIDIA_API_KEY=${process.env.NVIDIA_API_KEY}`)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 453 - 473, The env assignments are interpolated raw into the shell string passed to run(), allowing malicious values to inject shell code; update the creation of envArgs (used when building the command for run()) so each assignment is safely shell-quoted/escaped before joining (e.g., wrap each CHAT_UI_URL=... and NVIDIA_API_KEY=... assignment in proper single-quote/escaped form or use a shell-escaping helper) so the final command passed to run() cannot execute injected content; ensure the change is applied where envArgs is constructed and where run(...) is called (refer to envArgs, run(), and createResult/sandboxName).bin/lib/policies.js (1)
172-181:⚠️ Potential issue | 🟠 MajorReplace the
Date.now()temp path with a private temp directory.Line 173 still creates a predictable filename in a shared temp location. Another local process can pre-create or swap that path before
writeFileSync(), so the temp-file race this PR is trying to remove is still reachable here.🐛 Proposed fix
// Write temp file and apply - const tmpFile = path.join(os.tmpdir(), `nemoclaw-policy-${Date.now()}.yaml`); - fs.writeFileSync(tmpFile, merged, "utf-8"); + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-policy-")); + const tmpFile = path.join(tmpDir, "policy.yaml"); + fs.writeFileSync(tmpFile, merged, { encoding: "utf-8", mode: 0o600, flag: "wx" }); try { run(buildPolicySetCommand(tmpFile, sandboxName)); console.log(` Applied preset: ${presetName}`); } finally { - fs.unlinkSync(tmpFile); + fs.rmSync(tmpDir, { recursive: true, force: true }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/policies.js` around lines 172 - 181, The current code creates a predictable temp file via path.join(os.tmpdir(), `nemoclaw-policy-${Date.now()}.yaml`) which is vulnerable to TOCTOU; instead create a private temp directory with fs.mkdtempSync(path.join(os.tmpdir(), 'nemoclaw-policy-')) and write the YAML into a file inside that dir (e.g. path.join(tmpDir, 'policy.yaml')), then call run(buildPolicySetCommand(tmpFile, sandboxName)) as before, and in the finally block unlink the file and remove the temp directory (fs.rmdirSync or fs.rmSync) to ensure full cleanup; reference tmpFile/tmpDir, fs.mkdtempSync, writeFileSync(tmpFile, merged,...), buildPolicySetCommand, run, merged, presetName, sandboxName and adjust cleanup accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/nemoclaw.js`:
- Around line 161-165: The temporary secret file creation and cleanup around
envTmp and the scp upload are not exception-safe: wrap the upload call (the run
invocation that performs scp) in a try/finally so that fs.unlinkSync(envTmp) and
fs.rmdirSync(path.dirname(envTmp)) always run even if run(...) throws; locate
the block that constructs envTmp, writes it (fs.writeFileSync), calls run(`scp
... ${qname}:/home/ubuntu/nemoclaw/.env`) and move the unlinkSync/rmdirSync
calls into a finally block to guarantee removal of secrets on all failure paths.
- Around line 156-160: The code writes unescaped secret values into .env via the
envLines array (see envLines, getCredential) and later sources it, which allows
token content with newlines or shell metacharacters to be executed; fix by
escaping each value before adding to envLines (e.g., wrap values in single
quotes and escape any internal single quotes/newlines, or alternatively
serialize values using a safe format like base64 and decode when needed or stop
using shell `source` and use a dotenv parser), specifically update the logic
that builds envLines (including the NVIDIA_API_KEY line, GITHUB_TOKEN and
TELEGRAM_BOT_TOKEN entries) to perform proper shell-escaping/quoting of the
token strings before writing the file.
In `@test/runner.test.js`:
- Around line 154-168: The test "CLI rejects malicious sandbox names before
shell commands (e2e)" currently only asserts non-zero exit code which can mask a
regression where the injected "whoami" runs; update the test (in
test/runner.test.js) to also assert that the spawned process output does NOT
contain the current user name or typical whoami output by capturing
os.userInfo().username and checking that neither result.stdout nor result.stderr
includes that username (use spawnSync, result, stdout, stderr symbols) — this
proves the payload never executed in the child process.
---
Outside diff comments:
In `@bin/lib/onboard.js`:
- Around line 453-473: The env assignments are interpolated raw into the shell
string passed to run(), allowing malicious values to inject shell code; update
the creation of envArgs (used when building the command for run()) so each
assignment is safely shell-quoted/escaped before joining (e.g., wrap each
CHAT_UI_URL=... and NVIDIA_API_KEY=... assignment in proper single-quote/escaped
form or use a shell-escaping helper) so the final command passed to run() cannot
execute injected content; ensure the change is applied where envArgs is
constructed and where run(...) is called (refer to envArgs, run(), and
createResult/sandboxName).
In `@bin/lib/policies.js`:
- Around line 172-181: The current code creates a predictable temp file via
path.join(os.tmpdir(), `nemoclaw-policy-${Date.now()}.yaml`) which is vulnerable
to TOCTOU; instead create a private temp directory with
fs.mkdtempSync(path.join(os.tmpdir(), 'nemoclaw-policy-')) and write the YAML
into a file inside that dir (e.g. path.join(tmpDir, 'policy.yaml')), then call
run(buildPolicySetCommand(tmpFile, sandboxName)) as before, and in the finally
block unlink the file and remove the temp directory (fs.rmdirSync or fs.rmSync)
to ensure full cleanup; reference tmpFile/tmpDir, fs.mkdtempSync,
writeFileSync(tmpFile, merged,...), buildPolicySetCommand, run, merged,
presetName, sandboxName and adjust cleanup accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 794ed133-2aa7-444e-98bb-376a65730668
📒 Files selected for processing (9)
bin/lib/local-inference.jsbin/lib/nim.jsbin/lib/onboard.jsbin/lib/policies.jsbin/lib/runner.jsbin/nemoclaw.jsscripts/telegram-bridge.jstest/policies.test.jstest/runner.test.js
- Shell-quote secret values written to .env before remote source - Wrap scp upload in try/finally to guarantee temp secret cleanup - Shell-quote CHAT_UI_URL and NVIDIA_API_KEY env args in onboard - Replace predictable Date.now() temp path with mkdtempSync in policies - Strengthen e2e test with canary file to prove payload never executes - Fix merge-introduced test expectations for shellQuote single-quote format
Contributor
Author
|
All five findings addressed in 3832ec3:
Also fixed 4 test expectation mismatches from the main merge (double-quote → single-quote for |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bin/lib/policies.js (1)
31-36: Path traversal protection is effective, but has redundant condition.The
path.resolve+startsWithcheck correctly prevents path traversal attacks. However,file !== PRESETS_DIRis redundant since the filename always ends with.yamlsuffix (from`${name}.yaml`), making it impossible forfileto equalPRESETS_DIR.const file = path.resolve(PRESETS_DIR, `${name}.yaml`); - if (!file.startsWith(PRESETS_DIR + path.sep) && file !== PRESETS_DIR) { + if (!file.startsWith(PRESETS_DIR + path.sep)) { console.error(` Invalid preset name: ${name}`); return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/policies.js` around lines 31 - 36, In loadPreset, the path traversal guard redundantly checks "file !== PRESETS_DIR" even though file is constructed as `${name}.yaml`; remove that redundant comparison and keep the effective check using path.resolve and startsWith(PRESETS_DIR + path.sep) to validate the resolved file path (reference: function loadPreset and PRESETS_DIR).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bin/lib/policies.js`:
- Around line 31-36: In loadPreset, the path traversal guard redundantly checks
"file !== PRESETS_DIR" even though file is constructed as `${name}.yaml`; remove
that redundant comparison and keep the effective check using path.resolve and
startsWith(PRESETS_DIR + path.sep) to validate the resolved file path
(reference: function loadPreset and PRESETS_DIR).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8a31217c-a4e9-4be7-aa8f-7465e89a1d79
📒 Files selected for processing (5)
bin/lib/onboard.jsbin/lib/policies.jsbin/nemoclaw.jstest/onboard-readiness.test.jstest/runner.test.js
✅ Files skipped from review due to trivial changes (1)
- test/onboard-readiness.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- bin/lib/onboard.js
- bin/nemoclaw.js
Contributor
|
▎ Reviewed — this is solid. Centralizing shellQuote + validateName in ▎ I closed #540 in favor of this — yours covers -brian |
brianwtaylor
added a commit
to brianwtaylor/NemoClaw
that referenced
this pull request
Mar 21, 2026
Align with ericksoa's NVIDIA#584 approach: - Centralize shellQuote in runner.js, remove local copy from nemoclaw.js - Replace double-quoting with shellQuote() single-quote wrapping - Switch execSync to execFileSync in deploy() - Quote env values (API keys, tokens) with shellQuote - Max 63 chars (RFC 1123 label) instead of 253 (subdomain) - Update tests to verify shellQuote patterns
ericksoa
changed the title
Contributor
|
Reviewed the latest rebase onto What I checked:
Validation status:
Net: this is a real security improvement, not just churn, and it improves the codebase overall. Let it rip 🤙 |
kjw3
approved these changes
Mar 21, 2026
jnun
pushed a commit
to jnun/EasyClaw
that referenced
this pull request
Mar 21, 2026
) * security: fix command injection across all CLI entry points Comprehensive fix for shell injection vulnerabilities where user input (instance names, sandbox names, model names, API keys) was interpolated unsanitized into shell commands via run()/runInteractive()/execSync(). Changes: - Add shellQuote() and validateName() to runner.js as shared utilities - Replace all execSync() with execFileSync() in deploy (no shell) - Apply shellQuote() to every user-controlled variable in shell commands across nemoclaw.js, onboard.js, nim.js, policies.js - Add RFC 1123 name validation at CLI dispatch for sandbox/instance names - Fix path traversal in policies.js loadPreset() - Replace predictable temp file with mkdtempSync() - Remove duplicate shellQuote() definitions (now single source in runner.js) - 9 new test cases for shellQuote, validateName, and path traversal Supersedes NVIDIA#55, NVIDIA#110, NVIDIA#475, NVIDIA#540, NVIDIA#48, NVIDIA#171. * fix: deduplicate shellQuote in local-inference.js Import shellQuote from runner.js instead of defining a local copy. Single source of truth for shell quoting across the codebase. * security: fix telegram bridge injection + add regression guards Telegram bridge: - Replace execSync with execFileSync for ssh-config retrieval - shellQuote message, API key, and session ID in remote command - Validate SANDBOX_NAME at startup - Use mkdtempSync for temp SSH config (not predictable path) Regression tests: - nemoclaw.js must not use execSync - Single shellQuote definition in bin/ - CLI rejects malicious sandbox names (e2e, no mocking) - telegram-bridge.js validates SANDBOX_NAME and avoids execSync * security: address CodeRabbit review findings on shell injection PR - Shell-quote secret values written to .env before remote source - Wrap scp upload in try/finally to guarantee temp secret cleanup - Shell-quote CHAT_UI_URL and NVIDIA_API_KEY env args in onboard - Replace predictable Date.now() temp path with mkdtempSync in policies - Strengthen e2e test with canary file to prove payload never executes - Fix merge-introduced test expectations for shellQuote single-quote format
Closed
11 tasks
11 tasks
7 tasks
This was referenced Mar 22, 2026
4 tasks
Ryuketsukami
pushed a commit
to Ryuketsukami/NemoClaw
that referenced
this pull request
Mar 24, 2026
) * security: fix command injection across all CLI entry points Comprehensive fix for shell injection vulnerabilities where user input (instance names, sandbox names, model names, API keys) was interpolated unsanitized into shell commands via run()/runInteractive()/execSync(). Changes: - Add shellQuote() and validateName() to runner.js as shared utilities - Replace all execSync() with execFileSync() in deploy (no shell) - Apply shellQuote() to every user-controlled variable in shell commands across nemoclaw.js, onboard.js, nim.js, policies.js - Add RFC 1123 name validation at CLI dispatch for sandbox/instance names - Fix path traversal in policies.js loadPreset() - Replace predictable temp file with mkdtempSync() - Remove duplicate shellQuote() definitions (now single source in runner.js) - 9 new test cases for shellQuote, validateName, and path traversal Supersedes NVIDIA#55, NVIDIA#110, NVIDIA#475, NVIDIA#540, NVIDIA#48, NVIDIA#171. * fix: deduplicate shellQuote in local-inference.js Import shellQuote from runner.js instead of defining a local copy. Single source of truth for shell quoting across the codebase. * security: fix telegram bridge injection + add regression guards Telegram bridge: - Replace execSync with execFileSync for ssh-config retrieval - shellQuote message, API key, and session ID in remote command - Validate SANDBOX_NAME at startup - Use mkdtempSync for temp SSH config (not predictable path) Regression tests: - nemoclaw.js must not use execSync - Single shellQuote definition in bin/ - CLI rejects malicious sandbox names (e2e, no mocking) - telegram-bridge.js validates SANDBOX_NAME and avoids execSync * security: address CodeRabbit review findings on shell injection PR - Shell-quote secret values written to .env before remote source - Wrap scp upload in try/finally to guarantee temp secret cleanup - Shell-quote CHAT_UI_URL and NVIDIA_API_KEY env args in onboard - Replace predictable Date.now() temp path with mkdtempSync in policies - Strengthen e2e test with canary file to prove payload never executes - Fix merge-introduced test expectations for shellQuote single-quote format
jessesanford
pushed a commit
to jessesanford/NemoClaw
that referenced
this pull request
Mar 24, 2026
) * security: fix command injection across all CLI entry points Comprehensive fix for shell injection vulnerabilities where user input (instance names, sandbox names, model names, API keys) was interpolated unsanitized into shell commands via run()/runInteractive()/execSync(). Changes: - Add shellQuote() and validateName() to runner.js as shared utilities - Replace all execSync() with execFileSync() in deploy (no shell) - Apply shellQuote() to every user-controlled variable in shell commands across nemoclaw.js, onboard.js, nim.js, policies.js - Add RFC 1123 name validation at CLI dispatch for sandbox/instance names - Fix path traversal in policies.js loadPreset() - Replace predictable temp file with mkdtempSync() - Remove duplicate shellQuote() definitions (now single source in runner.js) - 9 new test cases for shellQuote, validateName, and path traversal Supersedes NVIDIA#55, NVIDIA#110, NVIDIA#475, NVIDIA#540, NVIDIA#48, NVIDIA#171. * fix: deduplicate shellQuote in local-inference.js Import shellQuote from runner.js instead of defining a local copy. Single source of truth for shell quoting across the codebase. * security: fix telegram bridge injection + add regression guards Telegram bridge: - Replace execSync with execFileSync for ssh-config retrieval - shellQuote message, API key, and session ID in remote command - Validate SANDBOX_NAME at startup - Use mkdtempSync for temp SSH config (not predictable path) Regression tests: - nemoclaw.js must not use execSync - Single shellQuote definition in bin/ - CLI rejects malicious sandbox names (e2e, no mocking) - telegram-bridge.js validates SANDBOX_NAME and avoids execSync * security: address CodeRabbit review findings on shell injection PR - Shell-quote secret values written to .env before remote source - Wrap scp upload in try/finally to guarantee temp secret cleanup - Shell-quote CHAT_UI_URL and NVIDIA_API_KEY env args in onboard - Replace predictable Date.now() temp path with mkdtempSync in policies - Strengthen e2e test with canary file to prove payload never executes - Fix merge-introduced test expectations for shellQuote single-quote format
This was referenced Mar 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Comprehensive fix for shell injection vulnerabilities where user input (instance names, sandbox names, model names, API keys) was interpolated unsanitized into shell commands via
run()/runInteractive()/execSync().Supersedes #55, #110, #475, #540, #48, #171.
Changes
bin/lib/runner.jsshellQuote()andvalidateName()as shared utilitiesbin/nemoclaw.jsexecSyncwithexecFileSyncin deploy; applyshellQuote()to all user input in shell commands; addvalidateName()at CLI dispatchbin/lib/onboard.jssetupInference(); remove duplicateshellQuote()bin/lib/nim.jsbin/lib/policies.jsshellQuote()in policy commands; path traversal fix inloadPreset()bin/lib/local-inference.jsshellQuote(); import from runner.jstest/runner.test.jsshellQuoteandvalidateNametest/policies.test.jsVulnerability classes fixed
nemoclaw deploy "test; rm -rf /"could execute arbitrary commands through 8+ unquoted SSH/rsync/scp/brev interpolationsopenshellcommands unquotedopenshell inference set --model ${model}was completely unquotedNVIDIA_API_KEYinterpolated into shell strings with double quotes (expandable)loadPreset("../../etc/passwd")could read arbitrary filesDate.now()replaced withmkdtempSync()Test results
32/32 unit tests pass. 14/14 integration tests pass including real bash round-trip verification with canary files proving no injection side effects.
Summary by CodeRabbit
Security
Tests